Skip to content

fix: ApplicationImpl.Begin raises SessionBegun after state changes (#5162)#5184

Closed
tig wants to merge 1 commit intodevelopfrom
fix/5162-cwp-session-begun
Closed

fix: ApplicationImpl.Begin raises SessionBegun after state changes (#5162)#5184
tig wants to merge 1 commit intodevelopfrom
fix/5162-cwp-session-begun

Conversation

@tig
Copy link
Copy Markdown
Member

@tig tig commented May 5, 2026

Fixes #5162.

Summary

  • ApplicationImpl.Begin violated the Cancellable Workflow Pattern (CWP): it invoked the SessionBegun event inside the session-stack lock and before calling SetIsRunning(true) / SetIsModal(true) on the runnable. Subscribers observed a SessionToken whose Runnable.IsRunning and IsModal were still false.
  • Move the state mutations (SetIsRunning(true), SetIsModal(true), previousTop?.SetIsModal(false)) inside the lock, and raise SessionBegun after the lock is released. This mirrors the existing symmetric End() / SessionEnded pattern.
  • Add a regression test in Tests/UnitTests.NonParallelizable that subscribes to SessionBegun and asserts IsRunning == true and IsModal == true on the event's SessionToken.Runnable.

The test was empirically verified to FAIL on develop (with the original ordering) and PASS after the fix. All 14 existing BeginEnd tests and the broader ApplicationTests suite (548 tests) continue to pass.

Test plan

  • New test SessionBegunCwpTests.SessionBegun_Raised_After_IsRunning_And_IsModal_Set_True fails on unfixed code (observed IsRunning == false)
  • New test passes after the fix
  • dotnet build succeeds
  • Existing BeginEndTests (14 tests) pass
  • Broader ApplicationTests namespace in UnitTestsParallelizable passes (548/548 + 2 pre-existing skips)

https://claude.ai/code/session_01CdoS4YQ7v9aNPZaNsYnxjv

Generated by Claude Code

@claude
fix: ApplicationImpl.Begin raises SessionBegun after state changes (#…
17ccb10 
…5162)

Per the Cancellable Workflow Pattern (CWP), state mutations must complete
before notifications. ApplicationImpl.Begin was invoking SessionBegun
inside the session-stack lock and BEFORE calling SetIsRunning(true) and
SetIsModal(true) on the runnable, so subscribers observed a SessionToken
whose Runnable.IsRunning and IsModal were still false.

Move the SetIsRunning/SetIsModal calls ahead of SessionBegun (still inside
the lock), and raise SessionBegun outside the lock after all state has
been committed. This matches the symmetric End()/SessionEnded pattern.

https://claude.ai/code/session_01CdoS4YQ7v9aNPZaNsYnxjv
@tig Claude
Copy link
Copy Markdown
Member Author

tig commented May 5, 2026

Closing this PR — on reflection the issue framing was wrong.

IsRunning is documented (IRunnable.cs:80) as a cached mirror of "is on SessionStack," and SessionBegun's XML doc only promises "Raised when Begin(IRunnable) has been called and has created a new SessionToken." The contract doesn't promise post-SetIsRunning ordering — it's a token-creation hook, not a "now running" notification. Subscribers who want post-state-change should listen to IsRunningChanged.

The original placement before SetIsRunning is consistent with that weak contract. The asymmetry with End()/SessionEnded (which fires last) is real but reflects an actual design distinction: "session created" vs. "session ended/disposed." This PR's fix re-asserts a stronger contract the public API never promised.

Closing #5184 unmerged. Issue #5162 is being closed wontfix in favor of a small doc-only PR that tightens the XML on SessionBegun/SessionEnded to make the hook semantics explicit.

Generated by Claude Code

@tig tig mentioned this pull request May 5, 2026
Closed
@tig tig closed this May 5, 2026
@tig tig mentioned this pull request May 5, 2026
Merged
1 task
tig pushed a commit that referenced this pull request May 5, 2026
@claude
docs: reframe #5162 as doc-only; reflect #5188 in tracker
236ff7b 
#5162 closed not_planned and PR #5184 closed unmerged after the CWP
framing was found to overstate the contract: SessionBegun is a
token-creation hook, SessionEnded is a token-disposal hook, and they
are deliberately not symmetric with the IsRunningChanged state-change
events. Replaced by doc-only PR #5188 tightening XML on IApplication.cs.

https://claude.ai/code/session_01CdoS4YQ7v9aNPZaNsYnxjv
@tig tig deleted the fix/5162-cwp-session-begun branch May 8, 2026 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CWP violation: ApplicationImpl.Begin() raises SessionBegun before setting IsRunning/IsModal

2 participants

@tig @claude